[arm_variant_type migration] Only expand arm_variant_type if defined#8293
[arm_variant_type migration] Only expand arm_variant_type if defined#8293traversaro wants to merge 1 commit intoconda-forge:mainfrom
Conversation
Without this fix, defining touch_arm_variant_type as suggested results in a re-rendering failure
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( I do have some suggestions for making it better though... For recipe/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/22525104535. Examine the logs at this URL for more detail. |
|
Is the related to undefined variables causing issues in rattler-build 0.58.*? |
Yeah, most likely... |
h-vetinari
left a comment
h-vetinari
left a comment
There was a problem hiding this comment.
There's some more specific syntax than the (IMO pretty hacky) touch_ resp. fake selector. Not as a criticism of this PR (which is fine IMO), but as a potential improvement.
| context: | ||
| # ensure arm_variant_type gets detected as a used variable | ||
| touch_arm_variant_type: ${{ arm_variant_type }} | ||
| touch_arm_variant_type: ${{ arm_variant_type if (linux and aarch64 and cuda_compiler_version != "None") else "None" }} |
There was a problem hiding this comment.
There's a cleaner way in v1 recipes that I hadn't been aware of until very recently, which might be preferable
build:
variant:
# ensure arm_variant_type gets detected as a used variable
use_keys:
- arm_variant_typeor, with the new, stricter rattler-build, probably also behind a if: linux and aarch64 and cuda_compiler_version != "None" guard.
| # A fake selector may be needed for conda-build to pick up arm_variant_type as a variant | ||
| # [arm_variant_type] | ||
|
|
||
| requirements: | ||
| build: | ||
| - {{ compiler('cuda') }} | ||
| - arm-variant * {{ arm_variant_type }} # [linux and aarch64 and cuda_compiler_version != "None"] |
There was a problem hiding this comment.
It even has an equivalent in v0 recipes that I've never seen in conda-forge (so we should test if it works):
build:
force_use_keys:
- arm_variant_typeIf so, it's probably more elegant than a fake selector...
There was a problem hiding this comment.
Ok for me, but I would like to know the opinion of @conda-forge/cuda and @carterbox on this.
There was a problem hiding this comment.
Now that using the default filter on undefined values is fixed for rattler build, I suggest:
- if: linux and aarch64 and not match(cuda_compiler_version, "==None")
then:
- arm-variant * ${{ arm_variant_type | default("None") }}There was a problem hiding this comment.
Or perhaps I don't understand the issue being addressed by this MR.
There was a problem hiding this comment.
Just to clarify, the comment of @h-vetinari was on an existing part of the migrator, not the part subject of the PR.
I am not sure, I saw that |
|
I just tested with rattler-build 0.58.4, and it is still failing the re-rendering in the current form: |
|
What about this? touch_arm_variant_type: ${{ arm_variant_type | default("None") }} |
|
I'm fine with using |
5 participants
Without this fix, defining touch_arm_variant_type as suggested results in a re-rendering failure.
fyi @conda-forge/cuda
Checklist
0(if the version changed)conda-smithy(Use the phrase@conda-forge-admin, please rerenderin a comment in this PR for automated rerendering)